Conversation
JBorrow
force-pushed
the
add-database-client
branch
2 times, most recently
from
947ec44 to
8f0879c
Compare
JBorrow
force-pushed
the
add-database-client
branch
from
8f0879c to
a22ee6f
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a synchronous, DB-backed SOCat client (parallel to the existing mock client) and refactors “sub-services” (astroquery/SSO/ephem) to be accessible as properties on a composite client. It also centralizes shared SQLModel/SQLAlchemy statements and session factory setup, and updates ingestion and tests to use the new client shape.
Changes:
- Add
socat/client/db.pyDB-backed composite client with.astroquery,.sso,.ephemsub-clients. - Introduce
socat/database/statements.pyand refactor core async update/query helpers to use shared SQL statements. - Centralize sync/async session factories and schema initialization in
socat/database/session.py, updating API imports and adding DB client tests.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_sso_mock.py | Refactors mock tests to use mock_client.sso / mock_client.ephem. |
| tests/test_sso_api.py | Adjusts API test expectations for cascade deletion behavior. |
| tests/test_mock.py | Updates mock tests to expect astroquery “no results” warnings and uses .astroquery sub-client. |
| tests/test_db_client.py | Adds coverage for DB-backed client CRUD, cascade behavior, and not-found semantics. |
| tests/conftest.py | Adds db_client fixture and refactors mock sub-client fixtures to derive from mock_client. |
| socat/ingest/jplparquet.py | Switches ingestion API to accept a composite client and changes mock-db builder implementation. |
| socat/database/statements.py | Adds shared SQL statement builders (create_name, box query, update helpers). |
| socat/database/session.py | New module for shared sync/async session factories and schema initialization. |
| socat/core/services.py | Refactors async update to use shared statements.update_service. |
| socat/core/moving_sources.py | Refactors async update to use shared statements.update_ephem. |
| socat/core/fixed_sources.py | Refactors box query and update to use shared statements. |
| socat/client/settings.py | Adds "db" backend option to client settings. |
| socat/client/mock.py | Adds .astroquery, .sso, .ephem properties to the mock composite client. |
| socat/client/db.py | New DB-backed composite client and sub-clients. |
| socat/client/core.py | Extends ClientBase with abstract .astroquery / .sso / .ephem properties. |
| socat/api/routers/*.py | Updates router imports to use centralized SessionDependency. |
| socat/api/async_ses.py | Removes duplicated async session dependency module. |
| socat/api/app.py | Uses centralized async schema initialization during FastAPI lifespan. |
| socat/api/init.py | Re-exports SessionDependency from the new centralized module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+22
to
+41
| def create_name( | ||
| name: str, astroquery_service: str | ||
| ) -> tuple[ICRS, str, Quantity | None]: | ||
| """""" | ||
| service: BaseVOQuery = getattr( | ||
| import_module(f"astroquery.{astroquery_service.lower()}"), | ||
| astroquery_service, | ||
| ) | ||
|
|
||
| requested_params = ["ra", "dec"] | ||
|
|
||
| result_table = service.query_object(name) | ||
| result_table["ra"].convert_unit_to("deg") | ||
| result_table["dec"].convert_unit_to("deg") | ||
| if "flux" in result_table.columns: | ||
| result_table["flux"].convert_unit_to("mJy") # pragma: no cover | ||
| result_dict = {param: None for param in requested_params} | ||
| if len(result_table) == 0: | ||
| return None | ||
| for param in requested_params: |
Collaborator
There was a problem hiding this comment.
Copilot is confused here but there does seem to be a type annotation error since create_name only raises an error the type annotation should be tuple[ICRS, str, Quantity].
Sulla2012
reviewed
Apr 27, 2026
Comment on lines
+22
to
+41
| def create_name( | ||
| name: str, astroquery_service: str | ||
| ) -> tuple[ICRS, str, Quantity | None]: | ||
| """""" | ||
| service: BaseVOQuery = getattr( | ||
| import_module(f"astroquery.{astroquery_service.lower()}"), | ||
| astroquery_service, | ||
| ) | ||
|
|
||
| requested_params = ["ra", "dec"] | ||
|
|
||
| result_table = service.query_object(name) | ||
| result_table["ra"].convert_unit_to("deg") | ||
| result_table["dec"].convert_unit_to("deg") | ||
| if "flux" in result_table.columns: | ||
| result_table["flux"].convert_unit_to("mJy") # pragma: no cover | ||
| result_dict = {param: None for param in requested_params} | ||
| if len(result_table) == 0: | ||
| return None | ||
| for param in requested_params: |
Collaborator
There was a problem hiding this comment.
Copilot is confused here but there does seem to be a type annotation error since create_name only raises an error the type annotation should be tuple[ICRS, str, Quantity].
Sulla2012
approved these changes
Apr 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a client interface that connects directly to the database.
Also refactors the core 'sub-services' to be properties of the client itself, so they are accessible in all cases.
Depends on #19.
The main change here is in
db.pywhere I've added a new interface that's similar tomockbut instead uses the more 'usable' database format. We will want to think very soon about how we ingest ephemeris here because it is currently very slow doing this on a row-by-row basis.This required taking some of the 'complex' databsae functionality and creating
statements.pythat lists shared statements that are beyond minor crud operations between the asynchonrous and synchronous interfaces. I am not yet fully convinced that this is the right final approach that we should keep but the alternative (re-using the asynchronous layer inside a synchronous wrapper) is also not idea.In addition I also centralized a bunch of database setup operations including making a mirror interface of the async system with its synchronous counterpart.
The new
db.pycomes along with tests for this new functionality.This PR used GPT-5.3-Codex through GitHub Copilot for some initial work. I will use the Copilot system to review the code now that it is complete.